Upgrade original targets #2942
Merged
Upgrade original targets #2942
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
peterebden
commented
Nov 2, 2023
| // Less returns true if this build label would sort less than another one. | ||
| func (label BuildLabel) Less(other BuildLabel) bool { | ||
| if label.PackageName == other.PackageName { |
There was a problem hiding this comment.
I'm restructuring this a bit but also it was wrong for ages because it didn't look at subrepos.
| @@ -59,7 +59,8 @@ func TestExpandVisibleOriginalTargets(t *testing.T) { | |||
|
|
|||
| func TestExpandOriginalSubLabels(t *testing.T) { | |||
| state := NewDefaultBuildState() | |||
| state.AddOriginalTarget(BuildLabel{PackageName: "src/core", Name: "..."}, true) | |||
There was a problem hiding this comment.
I don't think we ever actually needed to support this. The new implementation just panics but things like plz test //... are still fine - those are turned into a sequence of :all targets elsewhere.
Tatskaari
approved these changes
Nov 2, 2023
ChangeLog
Outdated
| @@ -11,6 +11,7 @@ Version 17.4.0 | |||
| * Add all commit date formats to supported git_show() format verbs (#2930) | |||
| * Fix reduce builtin (#2925) | |||
| * Fix issue with completing idents in the language server (#2917) | |||
| * Improve performance when a large number of input targets are supplied (#2942) | |||
There was a problem hiding this comment.
Indentation is a bit wonk here
Tatskaari
requested changes
Nov 2, 2023
src/core/state.go
Outdated
| } | ||
| return false | ||
| return state.progress.originalTargets.Match(target.Label) && state.ShouldInclude(target) |
There was a problem hiding this comment.
This logic isn't quite right. I think what we want is:
func (state *BuildState) isOriginalTarget(target *BuildTarget, exactOnly bool) bool {
allTargetsMatch, exact := state.progress.originalTargets.Match(target.Label)
return exact || (!exactOnly && allTargetsMatch && state.ShouldInclude(target))
}
Currently if I have a test with labels = ["manual"] and I match it exactly i.e. plz test //tests:manual, then it will be excluded because it's not an exact match.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've found cases where this can be notably slow when piping very large numbers of targets into plz. The code there is pretty old - when it was written we didn't have repos with hundreds of thousands of targets or remote execution with hundreds of workers, so we never really noticed.
This upgrades the implementation to be more efficient rather than running over a huge slice again and again.
I've tested using the profiler locally and things look much more sensible after this (i.e. not all the time is going into
OriginalTargetfunctions any more)